Skip to content

[stable29] multi-instance object store tweaks #57141

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
leftybournes wants to merge 6 commits into
base: stable29-backports
Choose a base branch
from
Draft

[stable29] multi-instance object store tweaks #57141

leftybournes wants to merge 6 commits into from

Conversation

@leftybournes
Copy link
Contributor

@leftybournes leftybournes commented Dec 17, 2025

No description provided.

icewind1991
icewind1991 reviewed Dec 17, 2025
View reviewed changes
@leftybournes leftybournes force-pushed the leftybournes/fix/stable29-backports branch from e7af913 to 8089fab Compare December 18, 2025 07:29
@leftybournes
feat: add move command
c295cfc 
Signed-off-by: Kent Delante <[email protected]>
@leftybournes leftybournes force-pushed the leftybournes/fix/stable29-backports branch from 8f6158a to 1a58a72 Compare December 19, 2025 05:14
@leftybournes leftybournes force-pushed the leftybournes/fix/stable29-backports branch from 1a58a72 to 32cb3bb Compare December 19, 2025 05:28
@icewind1991
Copy link
Member

icewind1991 commented Dec 19, 2025

I'm not sure if a move command like this is the correct option. (Only setting the stored instance, not moving an actual object). Or if we want to extend https://github.com/nextcloud/multibucket_migrate instead, which does perform the object moving.

icewind1991
icewind1991 reviewed Dec 19, 2025
View reviewed changes
@icewind1991
Copy link
Member

icewind1991 commented Dec 19, 2025

Outside of the move command the changes look good

@leftybournes
feat: add pre-migrate command
fc30176 
Signed-off-by: Kent Delante <[email protected]>
@leftybournes leftybournes force-pushed the leftybournes/fix/stable29-backports branch from 6b1a26e to fc30176 Compare December 23, 2025 07:39
@leftybournes
Copy link
Contributor Author

leftybournes commented Dec 23, 2025

I'm not sure if a move command like this is the correct option. (Only setting the stored instance, not moving an actual object). Or if we want to extend https://github.com/nextcloud/multibucket_migrate instead, which does perform the object moving.

I think extending multibucket_migrate is the way to go since that's already available instead of adding the move command.

github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 8, 2026
View reviewed changes
apps/files/lib/Command/Object/Multi/Move.php
->from('filecache')
->where($query->expr()->eq('storage', $query->createNamedParameter($storageId, IQueryBuilder::PARAM_INT)))
->andWhere($query->expr()->neq('mimetype', $query->createNamedParameter($folderMimetype, IQueryBuilder::PARAM_INT)));
$result = $query->execute();

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OCP\DB\QueryBuilder\IQueryBuilder::execute has been marked as deprecated
apps/files/lib/Command/Object/Multi/Move.php
->where($query->expr()->eq('storage', $query->createNamedParameter($storageId, IQueryBuilder::PARAM_INT)))
->andWhere($query->expr()->neq('mimetype', $query->createNamedParameter($folderMimetype, IQueryBuilder::PARAM_INT)));
$result = $query->execute();
$fileIds = $result->fetchAll(\PDO::FETCH_COLUMN);

Check notice

Code scanning / Psalm

PossiblyInvalidMethodCall Note

Cannot call method on possible int variable $result
icewind1991
icewind1991 reviewed Jan 8, 2026
View reviewed changes
icewind1991
icewind1991 reviewed Jan 8, 2026
View reviewed changes
@leftybournes leftybournes force-pushed the leftybournes/fix/stable29-backports branch from a3fbe3d to 84f7093 Compare January 9, 2026 07:59
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 9, 2026
View reviewed changes
icewind1991
icewind1991 requested changes Jan 9, 2026
View reviewed changes
Copy link
Member

@icewind1991 icewind1991 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from changing it to only verify a small number of objects for the move command this looks good to go

@leftybournes leftybournes force-pushed the leftybournes/fix/stable29-backports branch from 84f7093 to cd1d630 Compare January 12, 2026 08:16
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 12, 2026
View reviewed changes
icewind1991
icewind1991 reviewed Jan 12, 2026
View reviewed changes
icewind1991
icewind1991 reviewed Jan 12, 2026
View reviewed changes
@icewind1991
Copy link
Member

icewind1991 commented Jan 12, 2026

In general, I would suggest throwing exceptions in validateForUser for anything but the expected "object not found" error to prevent confusing users with an error message that doesn't match the error.

icewind1991
icewind1991 reviewed Jan 12, 2026
View reviewed changes
@leftybournes leftybournes force-pushed the leftybournes/fix/stable29-backports branch 2 times, most recently from 440a4e5 to ef7e00d Compare January 12, 2026 12:38
@leftybournes leftybournes force-pushed the leftybournes/fix/stable29-backports branch from ef7e00d to 7415781 Compare January 12, 2026 12:42
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 12, 2026
View reviewed changes
apps/files/lib/Command/Object/Multi/Move.php

$homeStorage = $homeMount->getStorage();
$storageId = $homeStorage?->getCache()->getNumericStorageId();
if ($storageId === null) {

Check notice

Code scanning / Psalm

DocblockTypeContradiction Note

int does not contain null
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@icewind1991 icewind1991 icewind1991 approved these changes

@skjnldsv skjnldsv Awaiting requested review from skjnldsv skjnldsv will be requested when the pull request is marked ready for review skjnldsv is a code owner

@Pytal Pytal Awaiting requested review from Pytal Pytal will be requested when the pull request is marked ready for review Pytal is a code owner

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl will be requested when the pull request is marked ready for review ArtificialOwl is a code owner

@come-nc come-nc Awaiting requested review from come-nc come-nc will be requested when the pull request is marked ready for review come-nc is a code owner

@artonge artonge Awaiting requested review from artonge artonge will be requested when the pull request is marked ready for review artonge is a code owner

@szaimen szaimen Awaiting requested review from szaimen szaimen will be requested when the pull request is marked ready for review szaimen is a code owner

@susnux susnux Awaiting requested review from susnux susnux will be requested when the pull request is marked ready for review susnux is a code owner

@nfebe nfebe Awaiting requested review from nfebe nfebe will be requested when the pull request is marked ready for review nfebe is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@leftybournes @icewind1991